Added sockHost, changed how sock URL is built #1858
Conversation
client-src/default/index.js
Outdated
| let sockHost = hostname; | ||
| let sockPath = '/sockjs-node'; | ||
| let sockPort = urlParts.port; | ||
| if (urlParts.path != null && urlParts.path !== '/') { |
There was a problem hiding this comment.
Please use strict equality operator. Or are you intentionally doing so to include undefined?
There was a problem hiding this comment.
That's how it was here: https://github.com/webpack/webpack-dev-server/blob/master/client-src/default/index.js#L235. I kept it that way under the assumption that it might need to catch undefined.
There was a problem hiding this comment.
I see. I didn't know that it was already used...
There was a problem hiding this comment.
@Loonride i think we should fix using != (==` and etc) in our code base
There was a problem hiding this comment.
@evilebottnawi should I fix it here?
There was a problem hiding this comment.
I want to change !==, but its change is the breaking change, so please rewrite as below.
if ((urlParts.path !== null || urlParts.path !== undefined) && urlParts.path !== '/') {
There was a problem hiding this comment.
I think urlParts.path != null would turn into (urlParts.path !== null && urlParts.path !== undefined)
lib/options.json
Outdated
| "type": "string" | ||
| }, | ||
| { | ||
| "type": "null" |
There was a problem hiding this comment.
I think we don't have to write explicitly.
There was a problem hiding this comment.
You mean the "type": "null"? The host option has the same format.
There was a problem hiding this comment.
Yes, we discussed here, and I also don't think it's necessary.
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
As mentioned here: #1777, there could potentially be a need for a
sockHostwhich is different from thehostoption.I also realized that when
sockPortwas added, a problem could occur ifsockPortwas included butsockPathwas not. Specifically, they both checkurlParts.path === '/'to see if the query string should be parsed or not, but that method of checking only works if a single option is passed in throughurlParts.path. I can make a minimum reproducible test repo for this problem if needed. This is the reasoning behind partially changing how the sock url is built.Breaking Changes
None
Additional Info